Conversation
Under the hood ObjectV3 endpoint is used. This is initial shape, and we are expecting Payloads to change in the future.
Under the hood ObjectV3 endpoint is used. This is initial shape, and we are expecting Payloads to change in the future.
Under the hood ObjectV3 endpoint is used. This is initial shape, and we are expecting Payloads to change in the future.
Under the hood ObjectV3 endpoint is used. This is initial shape, and we are expecting Payloads to change in the future.
Under the hood ObjectV3 endpoint is used. This is initial shape, and we are expecting Payloads to change in the future.
Under the hood ObjectV3 endpoint is used. This is initial shape, and we are expecting Payloads to change in the future.
Under the hood ObjectV3 endpoint is used. This is initial shape, and we are expecting Payloads to change in the future.
Under the hood ObjectV3 endpoint is used. This is initial shape, and we are expecting Payloads to change in the future.
Under the hood ObjectV3 endpoint is used. This is initial shape, and we are expecting Payloads to change in the future.
Under the hood ObjectV3 endpoint is used. This is initial shape, and we are expecting Payloads to change in the future.
Under the hood ObjectV3 endpoint is used. This is initial shape, and we are expecting Payloads to change in the future.
Under the hood ObjectV3 endpoint is used. This is initial shape, and we are expecting Payloads to change in the future.
Under the hood ObjectV3 endpoint is used. This is initial shape, and we are expecting Payloads to change in the future.
kleewho
left a comment
There was a problem hiding this comment.
Some of the comments are applicable to more places
|
|
||
| import java.util.Map; | ||
|
|
||
| public abstract class CreateUser extends UserEndpoint<CreateUser, EntityEnvelope<User>, CreateUserResult> implements CustomIncludeAware<CreateUser> { |
There was a problem hiding this comment.
This could be an interface. It'll save us few lines. This requires few additional changes in generic parameter boundaries from Endpoint to RemoteAction. This how the full interface would look like:
public interface CreateUser extends RemoteAction<CreateUserResult>, CustomIncludeAware<CreateUser> {
CreateUser name(String name);
CreateUser email(String email);
CreateUser profileUrl(String profileUrl);
CreateUser externalId(String externalId);
CreateUser custom(Map<String, Object> custom);
CreateUser status(String status);
CreateUser type(String type);
CreateUser userId(UserId userId);
static CreateUser create(
final PubNub pubNub,
final TelemetryManager telemetryManager,
final RetrofitManager retrofitManager,
final TokenManager tokenManager) {
final CompositeParameterEnricher compositeParameterEnricher = CompositeParameterEnricher.createDefault();
return new CreateUserCommand(pubNub, telemetryManager, retrofitManager, tokenManager, compositeParameterEnricher);
}
}
There was a problem hiding this comment.
I will contact you to talk about this change.
There was a problem hiding this comment.
I tried to introduce interface but is seems more easier approach to reduce code we can get rid of CreateUserCommand class and move all login from that class to CreateUser that will be changed from abstract to non abstract class. I also removed UserEndoint class.
| private String externalId; | ||
| private Map<String, Object> custom; | ||
| private String status; | ||
| private String type; |
There was a problem hiding this comment.
All those fields could have generated fluent setters with lombok, just like we have for most of other implementations. It'll save us few lines. It'll look like this:
@Accessors(chain = true, fluent = true)
public final class CreateUserCommand extends CreateUser implements HavingCustomInclude<CreateUser> {
@Setter
private String name;
There was a problem hiding this comment.
I can change it without big enthusiasm. With this construction code is more difficult to read for people that don't know lombok. But this is good chance for me to learn it :)
FYI
In SetChannelMetadataCommand and SetUUIDMetadataCommand this lombok is not used.
There was a problem hiding this comment.
I'm not a fan of lombok either, but if we have it already and it's used in quite a few places we can as well just make use of it
There was a problem hiding this comment.
In SetChannelMetadataCommand and SetUUIDMetadataCommand this lombok is not used.
That might have been an oversight. AFAIR it was a big big PR as well, so I might have missed something
| import java.util.HashMap; | ||
| import java.util.Map; | ||
|
|
||
| public final class CreateUserCommand extends CreateUser implements HavingCustomInclude<CreateUser> { |
There was a problem hiding this comment.
This is an implementation so I'd make it package private. There should be no need for customer to use these
There was a problem hiding this comment.
I like this idea.
| return SetChannelMetadata.builder(this, this.telemetryManager, this.retrofitManager, this.tokenManager); | ||
| } | ||
|
|
||
| public CreateSpace createSpace() { |
There was a problem hiding this comment.
I would likely see here all required parameters instead of everything being optional. That would mean that all space related methods should have spaceId at least. This could bring additional changes in other places: making it required in the *SpaceCommand and also most probably the SpaceEndpoint will no longer be necessary
There was a problem hiding this comment.
I like this idea.
There was a problem hiding this comment.
I knew that you'll like it :)
| protected UserId effectiveUserId() { | ||
| try { | ||
| return (userId != null) ? userId : getPubnub().getConfiguration().getUserId(); | ||
| } catch (PubNubException e) { |
There was a problem hiding this comment.
Seeing this catch I think we made a mistake by keeping uuid in configuration as a base field instead of UserId. We wouldn't have to have this here for simple getUserId
|
|
||
| @NoArgsConstructor | ||
| public class FetchSpaceResult extends SpaceResult { | ||
| public FetchSpaceResult(EntityEnvelope<Space> entityEnvelope) { |
| public interface SpaceService { | ||
|
|
||
| @POST("/v3/objects/{subKey}/spaces/{spaceId}") | ||
| @Headers("Content-Type: application/json; charset=UTF-8") |
There was a problem hiding this comment.
Shouldn't we include it everywhere where we have Body or not include it at all?
There was a problem hiding this comment.
I will delete it.
I found @headers in ChannelMetadataService and UUIDMetadataService so I will delete them from there as well.
| @Getter | ||
| @ToString | ||
| @AllArgsConstructor | ||
| public abstract class UserPayload { |
There was a problem hiding this comment.
I'd use just @Data instead of these annotations
There was a problem hiding this comment.
@Data introduces @Setter. I am wondering if we want to have setters for those fields?
There was a problem hiding this comment.
it doesn't if you have everything final
| @Getter | ||
| @ToString | ||
| @AllArgsConstructor | ||
| public abstract class SpacePayload { |
There was a problem hiding this comment.
@Data should be all right here instead of currently present
There was a problem hiding this comment.
Makes sense. I will change it.
|
|
||
| import java.util.Map; | ||
|
|
||
| public abstract class CreateSpace extends SpaceEndpoint<CreateSpace, EntityEnvelope<Space>, CreateSpaceResult> implements CustomIncludeAware<CreateSpace> { |
There was a problem hiding this comment.
Why the return value is CreateSpaceResult and not just Space? In Kotlin we return just Space. Could you please align the return values for all new methods to what we have in Kotlin already?
There was a problem hiding this comment.
I forgot that in Kotlin we get rid of status in response. I will change CreateSpaceResult to Space. Status might be inferred by Space object being present or exception being thrown
e.g. when creating space with id that already exists following exception is thrown:
{"error":{"message":"Requested resource already exists.","source":"metadata"},"status":409}
kleewho
left a comment
There was a problem hiding this comment.
The ObjectsV2 implementation is one of the most complicated things in the JavaSDK and I think we went over the top with this. I don't think it needs to be as complicated. I'm not sure what you think about it, but feel free to drop all the complicated bits of that implementation if you share this impression
| @EqualsAndHashCode(callSuper = true) | ||
| @ToString(callSuper = true) | ||
| @NoArgsConstructor | ||
| public class Space extends PNObject { |
There was a problem hiding this comment.
Shouldn't we have a SpaceId as a type for id field for this class?
There was a problem hiding this comment.
Yes. Good catch. I will create class similar to PNObject but with no id and add:
private SpaceId id;
to Space.java.
I will also add custom interceptor to map id as String in REST response to SpaceId .
public class SpaceIdDeserializer implements JsonDeserializer {
@SneakyThrows
@OverRide
public SpaceId deserialize(JsonElement json, Type typeOfT, JsonDeserializationContext context) throws JsonParseException {
String spaceIdValue = json.getAsString();
return new SpaceId(spaceIdValue);
}
}
| @EqualsAndHashCode(callSuper = true) | ||
| @ToString(callSuper = true) | ||
| @NoArgsConstructor | ||
| public class User extends PNObject { |
There was a problem hiding this comment.
Same story as with Space. I think its id should have type UserId
There was a problem hiding this comment.
I will add it. Thanks.
| @Setter(AccessLevel.PACKAGE) | ||
| @NoArgsConstructor | ||
| @ToString(callSuper = true) | ||
| public class RemoveSpaceResult extends EntityEnvelope<JsonElement> { |
There was a problem hiding this comment.
Simpler alternative
@Data
public class RemoveSpaceResult {
private final int status;
}
There was a problem hiding this comment.
Ok. I will use magic of lombok.
| } | ||
|
|
||
| @Override | ||
| public PNObject setCustom(Object custom) { |
There was a problem hiding this comment.
I think custom should not be an Object but rather should have the same type as we expect when we createSpace, so Map<String, Object>.
What would you say about this simpler version of Space?
@Data
public class Space {
@NonNull
private final SpaceId id;
private final String name;
private final String description;
private final String status;
private final String type;
private final Map<String, Object> custom;
}
probably we'd need custom deserializer for SpaceId
There was a problem hiding this comment.
I think this is good improvement. I guess we need to add eTag and updated field so:
@EqualsAndHashCode(onlyExplicitlyIncluded = true)
@DaTa
public class Space extends ObjectVsp {
@EqualsAndHashCode.Include
@JsonAdapter(SpaceIdDeserializer.class)
@Setter
private SpaceId id;
private String name;
private String description;
private String status;
private String type;
@Override
public Space setCustom(Map<String, Object> custom) {
super.setCustom(custom);
return this;
}
}
@DaTa
@accessors(chain = true)
public class ObjectVsp {
@JsonAdapter(CustomPayloadJsonInterceptor.class)
@Setter
protected Map<String, Object> custom;
protected String updated;
protected String eTag;
}
| if (input.body() != null) { | ||
| return input.body().getData(); | ||
| } else { | ||
| return new Space(); |
There was a problem hiding this comment.
I think it should rather be an error. We are expecting body in case of successful response
There was a problem hiding this comment.
Not sure what you suggest? Something like this ?
if (input.body() != null) {
return input.body().getData();
} else {
throw PubNubException.builder().pubnubError(PubNubErrorBuilder.PNERROBJ_CHANNEL_MISSING).build();
}
In case of error program will not hit this method. So return new Space() should never happen.
I was looking at PNSetChannelMetadataResult, GetChannelMetadataCommand and there empty object creation but maybe it's more precise to throw exception.
Removed *Command classes Removed UserEndpoint classes Made CreateSpace, FetchSpace..., CreateUser, FetchUser... non abstract
This reverts commit a7e1969.
Introduced interfaces for all User/Space operations e.g. CreateUser, CreateSpace
1.No sure if we want to put deprecation messages on ObjectV2 method just now because ObjectV3 REST API doesn't seem to be in final shape.
2.I realised that ObjectV3 Rest API doesn't return
updatedandeTagfields.